I know this is a pre-existing choice, but It may make sense to allow for more dimensions? I.e. this would be a case where we have something like

   >>> states = ct.SolutionArray(gas, (6, 10), extra=[‘foo’, ‘bar’])

I can't clearly understand what you want to say. I guess you want to say foo and bar to be extra arrays of size (6,10)?
like

>>> states = ct.SolutionArray(gas, (6,10), extra={'foo':sd,"bar":sd})

where sd is any (6,10) list/np array
??

@sin-ha ... what I wrote was deliberate - no typo there. It currently throws a ValueError, but you could argue that it should initialize np.ndarray's with the full _shape, not just the first dimension. From my perspective, the shape of the _extra entries should be consistent with other data.

@ischoegl I'm also not sure what you're suggesting, but it seems like it would be a new feature? In that case, I think it seems outside the scope of this fix.

@ischoegl As of now extra expects dicts unless the shape of SolutionArray is trivial

• If the user passes a list we should be able to initiate the extra array with same shape.
• else the user will pass that as a dict , in that case we should validate extra with the complete SolutionArray shape instead of self._shape[0]

I also do think this is more of adding another functionality, seems a bit off of the issue so should it be pulled here or open another issue?

I think the point I raised is a trivial addition, and the application was stated in my first post: it’s a reasonable assumption that the arrays are instantiated with correct dimensions based on the information already provided. My guess is that the former single-dimensioned approach was based on _extra formerly being list-based. Now that this is no longer the case, putting in a more consistent approach is trivial.

What I am talking about affects some of the same lines that are changed already and won’t take more than a couple of additional lines. Hence, I’d like to see it considered as a friendly amendment of the original issue 😉

That's captures almost everything! However, extra could also be an instance of tuple or ndarray of strings? I believe all of these are supported for when no shape is used for the instantiation of SolutionArray (i.e. I am talking about the former elif extra and self._shape == (0,): case, in which case any of the above can be iterated over).

that'd be a quick fix but I don't think np.arrays are important?.
Besides I believe the other part should be removed where self._shape is trivial as after this commit it would only be a desperate attempt to map something in extra, probably not useful (is there a particular use case?). it lacks checks and something weird like integers, for instance can pass which can't be used back.e.g.

>>>>states3 = ct.SolutionArray(gas, shape=(0,0,0), extra=[1])

I don’t think that the alternative case can be executed for useful content either (you already caught all useful cases), I.e. I believe it can be deleted. If I’m not mistaken, the unit tests should still pass.

On a formatting note, there are a couple of spaces missing after commas (e.g. it should be shape=(0, 0, 0) in the previous post; same for some other lines you edited). It’s minor, but the code should remain in compliance with PEP 8.

PS: ad ndarrays ... again it’s minor but it should be included for consistency.

Yes the unit tests pass.
sorry for the formatting though.

But this isn't correct, np.append does not work in-place. See the documentation: https://numpy.org/doc/1.18/reference/generated/numpy.append.html This will most likely be very slow, since NumPy allocates a new array every time np.append() is called. I wonder if it would be better to cast the values in _extra to lists and use the list append method, and then cast them back to arrays? Maybe the complexity isn't worth it though.

I also can't find any other way around see

What do you mean, any other way around? You'll have to find some way, since this won't work as expected. I think the simplest method is just to reassign value: value = np.append(value, kwargs.pop(name)) That might be slow, since NumPy has to create a new array every time, but hopefully not too slow.

This line still isn't fixed.

I didn't think this through before, Anyways

I think the simplest method is just to reassign value: `value = np.append(value, kwargs.pop(name))

Appending np.ndarrays is not as fast

Turns out converting them to lists and appending (as you said before). does a better job!!,

see https://stackoverflow.com/a/22394181 and https://stackoverflow.com/questions/7133885/fastest-way-to-grow-a-numpy-numeric-array

In [2]: %%timeit 
   ...: a = np.empty((0,3), int) 
   ...: l = list(a) 
   ...: for i in range(1000): 
   ...:     l.append([3*i+1,3*i+2,3*i+3]) 
   ...: l = np.array(l)                                                                                                                                                                                                   
467 µs ± 6.32 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

In [3]: %%timeit 
   ...: a = np.array([3,4,3]) 
   ...: for i in range(1000): 
   ...:     l = list(a) 
   ...:     l.append([3*i+1,3*i+2,3*i+3]) 
   ...:     l = np.array(l) 
   ...:                                                                                                                                                                                                    
2.27 ms ± 531 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

In [4]: %%timeit 
   ...: a = np.empty((0,3), int) 
   ...: for i in range(1000): 
   ...:     a = np.append(a, 3*i+np.array([[1,2,3]]), 0) 
   ...:                                                                                                                                                                                                    
3.88 ms ± 21.2 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

I don't the 'extra' arrays should be allowed to contain uninitialized values. The existing behavior of requiring initial values if the SolutionArray is not empty should be preserved.

@speth ... I’d ask to consider retaining this behavior (while potentially avoiding np.empty).

Think of a scenario where extra is calculated based on the content of the SolutionArray itself (which typically receives the actual value using some setter only after it is instantiated, so the content of extra is not known a priori). For this case, you’d have to provide dummy values to create extra columns, which would be subsequently overwritten. I believe that automatically initializing values as zeros would be a lot more convenient (and avoid np.empty); columns that contain strings are presumably rare, so having to provide a value there would be ok.

@ischoegl I originally had the same objection as you, but I considered it for a while and I ended up agreeing with @speth. There are two main reasons for this, as I see it:

1. It is trivially easy to specify a dummy value, since you do not need to specify the shape. For instance:

   >>> arr = ct.SolutionArray(gas, (3, 10), extra={"prop": 0, "prop2": 0.0})
   >>> arr.prop
   array([[0, 0, 0, 0, 0, 0, 0, 0, 0, 0],
          [0, 0, 0, 0, 0, 0, 0, 0, 0, 0],
          [0, 0, 0, 0, 0, 0, 0, 0, 0, 0]])
   >>> arr.prop2
   array([[0., 0., 0., 0., 0., 0., 0., 0., 0., 0.],
          [0., 0., 0., 0., 0., 0., 0., 0., 0., 0.],
          [0., 0., 0., 0., 0., 0., 0., 0., 0., 0.]])
   >>> # This is not allowed right now, but would be almost the same, except the dtype
   >>> arr = ct.SolutionArray(gas, (3, 10), extra=["prop", "prop2"])

   Since I've used np.full() to fill the initial value, there's no need to give an array as the initial value.

2. It isn't clear what dtype should be used - it is not only strings, but whether one wants to use floats vs. ints or any other dtype as well. I suppose floats could be used in place of ints, but there are legitimate use cases for the int dtype, for instance, as a categorical or level variable. I think it would also not be clear why one would have to specify an initial value for some dtypes but not for others.

@bryanwweber ... I didn’t think of the scalar route, so your argument is convincing.

The availability of the scalar initializer is a nice compromise for the case where the value is going to be calculated anyway.

If I'm reading this block of code correctly now, the only time that this line can be called is if self._shape is (0,), right? It might be more clear if the value was just set to self._extra[name] = np.empty(0) then.

Yes, that's correct, it only runs if self._shape == (0,). I'm not sure it's clearer to just pass 0 though. Shapes of NumPy arrays are always returned as tuples, and just writing np.empty(0) implies to me that 0 will be the value in the array (notwithstanding the name empty), not that there's a shape of 0. So I think it would be better to use np.empty(shape=(0,)), which is then just self._shape, and putting in (0,) I thought would be more confusing than using self._shape, because the question arises, if these are equal why not just use the existing variable name?

I think explicitly showing the size as zero (with whatever notation you like) is preferable because it makes it clear that this doesn't result in an array with uninitialized values. Tracing the logic back to see that self._shape is (0,) if we're in this branch is not really that simple.

OK, this is fixed now with shape=(0,) 👍